Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Delegate date strings formatting to datetime.isoformat() #6413

Merged
merged 1 commit into from
Dec 20, 2024
Merged

Delegate date strings formatting to datetime.isoformat() #6413

merged 1 commit into from
Dec 20, 2024

Conversation

gonzalo-bulnes
Copy link
Contributor

@gonzalo-bulnes gonzalo-bulnes commented Apr 27, 2022 •
edited by cfm
Loading

Status

Ready for review

Description of Changes

Closes #6257 (see Proposed plan section in the description).

Changes proposed in this pull request:

  • Remove the custom date string format (that used to be the only one supported by the SecureDrop Client)
  • Use ISO8061 via datetime.isoformat() instead (should be easier to use consistently too)

Testing

  • Visual review: do these changes make sense given the context recapped below?

Then test here:

  • Pay special attention to CI: do the tests changed here still pass?

And then check for surprises downstream:

  • Run make dev here and make regenerate-sdk-cassettes in freedomofpress/securedrop-client/client. It should pass.
  • Run make test in freedomofprses/securedrop-client/client. It should pass.

Deployment

Once this change has been released, the try/except logic in https://github.com/freedomofpress/securedrop-client/blob/7f7f60e67f7606708a6c9ef519c9657cd738be9e/client/securedrop_client/sdk/timestamps.py can be removed in favor of just datetime.fromisoformat().

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

@gonzalo-bulnes gonzalo-bulnes force-pushed the delegate-tz-string-formatting-to-datetime branch from 1eff662 to 1aaa7b3 Compare April 27, 2022 21:41
@gonzalo-bulnes
Copy link
Contributor Author

@legoktm I'm trying to gather again the context around this PR. It seems like one of the API responses contains timestamps without time zone information (specific error in CI). I believe that's a legit test suite error and that we want to fix the API response to conform to the new convention, does that seem right to you?

@gonzalo-bulnes @legoktm @cfm
Delegate date strings formatting to datetime.isoformat()
da8f43f 
The SecureDrop SDK now supports date stings in ISO8061 format,
so we can stop relying on custom-defined formats.

The custom JSON formatter made this a one-line change!

Co-authored-by: Kunal Mehta <[email protected]>
@cfm cfm force-pushed the delegate-tz-string-formatting-to-datetime branch from 1aaa7b3 to da8f43f Compare December 5, 2024 20:23
@cfm cfm marked this pull request as ready for review December 5, 2024 20:37
@cfm cfm requested a review from a team as a code owner December 5, 2024 20:37
@cfm
Copy link
Member

cfm commented Dec 5, 2024

Tl;dr: This change does not have any side effects on time-zone support. It simply standardizes us to datetime.isoformat() and should be either approved or closed based whether we currently think that standardization is worthwhile.

We decided in today's backlog-pruning session that this was worth revisiting in a time-box to see if we could finalize it. Substantively all I've done is rebase this from develop. But to recap the context here:

  1. securedrop currently sends UTC timestamps in a custom API_DATETIME_FORMAT.
  2. With this change, securedrop will send UTC timestamps exclusively in datetime.isoformat().
  3. securedrop-client, specifically the SDK, will already parse either (1) or (2). (The test plan above confirms this.)
  4. After this change has been released, securedrop-client can drop the trial-and-error parsing in (3).

@cfm cfm requested a review from legoktm December 5, 2024 20:38
@legoktm legoktm self-assigned this Dec 11, 2024
legoktm
legoktm approved these changes Dec 20, 2024
View reviewed changes
Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; I tested that the current client can handle the new ISO timestamp format just fine.

@legoktm legoktm added this pull request to the merge queue Dec 20, 2024
Merged via the queue into develop with commit 0ba1208 Dec 20, 2024
45 checks passed
@legoktm legoktm deleted the delegate-tz-string-formatting-to-datetime branch December 20, 2024 20:10
@gonzalo-bulnes
Copy link
Contributor Author

Wow. That was some time ago. 😮

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legoktm legoktm legoktm approved these changes

Assignees

@legoktm legoktm

Labels
None yet
Projects
SecureDrop dev cycle
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

API responses should include time-zone information, be valid ISO8061/RFC339
3 participants
@gonzalo-bulnes @cfm @legoktm